gh-115957: Close coroutine if the TaskGroup is inactive #116009

Jason-Y-Z · 2024-02-27T21:55:59Z

This PR addresses the issue where when we call TaskGroup.create_task() with an inactive TaskGroup, we still get a RuntimeWarning for the coroutine. This change makes sure that the coroutine is closed in this case and updated the tests accordingly.

📚 Documentation preview 📚: https://cpython-previews--116009.org.readthedocs.build/

Issue: asyncio.TaskGroup does not close unawaited coroutines #115957

arthur-tacca · 2024-02-28T14:27:52Z

Thanks for doing this! I was dangerously close to doing it myself even though I really don't have time, just for the kudos of being a "Python contributor", so I'm really glad you did 😄

My comments:

You have described this as affecting "...if the task group has been closed" but it also affects task groups that have not yet been entered and those that are shutting down (but not yet closed). Perhaps the wording should be something like "when create_task is called on a TaskGroup that is not active (because it has not yet been entered, is shutting down or has closed)"
I could be mistaken but I think that test would pass even without your change? It looks like it tests that asyncio.TaskGroup.create_task() will raise RuntimeError if the task group has closed, but it already did that. To test the actual change it would need to check that the coroutine has been closed (by calling coro.send(None) and checking that gives RuntimeError: cannot reuse already awaited coroutine).

Jason-Y-Z · 2024-02-28T15:29:55Z

Thanks for doing this! I was dangerously close to doing it myself even though I really don't have time, just for the kudos of being a "Python contributor", so I'm really glad you did 😄

My comments:

You have described this as affecting "...if the task group has been closed" but it also affects task groups that have not yet been entered and those that are shutting down (but not yet closed). Perhaps the wording should be something like "when create_task is called on a TaskGroup that is not active (because it has not yet been entered, is shutting down or has closed)"

I could be mistaken but I think that test would pass even without your change? It looks like it tests that asyncio.TaskGroup.create_task() will raise RuntimeError if the task group has closed, but it already did that. To test the actual change it would need to check that the coroutine has been closed (by calling coro.send(None) and checking that gives RuntimeError: cannot reuse already awaited coroutine).

Thanks for the comments Arthur! Yeah that makes sense, I will update the wording there.

Regarding the test, it raises a RuntimeWarning without the change for me. Maybe we have slightly different configs? But yeah let me try more specifically sending on the coroutine as well. Thanks again!

arthur-tacca · 2024-02-28T15:38:09Z

Sorry, to be honest I haven't actually run the tests (or for that matter cloned the CPython repo at all), I was just reading the test code. If the test suite automatically fails if there's a warning then you can ignore that comment.

Jason-Y-Z · 2024-02-29T17:30:53Z

Sorry for tagging but a gentle nudge for reviewing please 🙏 @gvanrossum

gvanrossum · 2024-03-01T19:41:03Z

Sorry for tagging but a gentle nudge for reviewing please 🙏 @gvanrossum

Yeah, it's in my queue.

Jason-Y-Z · 2024-03-01T22:56:32Z

Sorry for tagging but a gentle nudge for reviewing please 🙏 @gvanrossum

Yeah, it's in my queue.

Got it, thank you 🙏

gvanrossum

It looks like the tests you changed and added still pass even if I comment out the three coro.close() calls added to taskgroups.py. While they print the RuntimeWarning message, they don't fail. Could you try to detect the warnings in at least one of the tests?

gvanrossum · 2024-03-05T01:09:02Z

Doc/library/asyncio-task.rst

+      .. versionchanged:: 3.13
+
+         Close the given coroutine if the task group is not active.
+


I believe that the convention is that a description of the new/changed behavior is added to the main text of the documentation (in this case the two lines right above) and then the versionchanged directive is used to clarify when this behavior was added or changed. In this case that would be a little redundant but I still feel it's appropriate.

For example, the main text could include information about when a task group is considered inactive (e.g. not yet entered, already finished, or in the process of shutting down).

Thanks for the explanation! I've updated the main text

gvanrossum · 2024-03-05T01:12:11Z

Doc/whatsnew/3.13.rst

@@ -185,6 +185,11 @@ Other Language Changes

  (Contributed by Sebastian Pipping in :gh:`115623`.)

+* When :func:`asyncio.TaskGroup.create_task` is called on an inactive
+  :class:`asyncio.TaskGroup`, the given coroutine will be closed (which
+  prevents a :exc:`RuntimeWarning`).


I'd clarify that the RuntimeWarning is about "coroutine 'blank' was never awaited".

Thanks for the comments! I've updated the wording

bedevere-app · 2024-03-05T01:17:14Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Jason-Y-Z · 2024-03-05T09:26:02Z

It looks like the tests you changed and added still pass even if I comment out the three coro.close() calls added to taskgroups.py. While they print the RuntimeWarning message, they don't fail. Could you try to detect the warnings in at least one of the tests?

Thanks for the detailed explanations @gvanrossum ! I have updated one of the tests so that it now explicitly captures and asserts no warning. I've tried it on my machine (MacOS) and it is now failing without the change. Please let me know if there is any other improvement I can apply. Thanks!

I have made the requested changes; please review again

bedevere-app · 2024-03-05T09:26:07Z

Thanks for making the requested changes!

@gvanrossum: please review the changes made to this pull request.

gvanrossum

Thanks, looks good! The tests pass in CI, one of the tests fails if I undo the changes in taskgroups.py only, so I will merge.

gvanrossum · 2024-03-06T20:19:37Z

PS. Next time please don't force push. We squash changes when we merge into main anyway, and the force push makes it harder to re-review the code.

Jason-Y-Z · 2024-03-07T23:32:19Z

Thanks @gvanrossum for the review and for the note! I will remember not to force push next time 👌

… error (python#116009)

Jason-Y-Z requested review from 1st1, asvetlov, gvanrossum, kumaraditya303 and willingc as code owners February 27, 2024 21:56

bedevere-app bot added the awaiting review label Feb 27, 2024

bedevere-app bot mentioned this pull request Feb 27, 2024

asyncio.TaskGroup does not close unawaited coroutines #115957

Closed

Jason-Y-Z force-pushed the fix-issue-115957 branch 2 times, most recently from e4ab02f to 6a0265d Compare February 27, 2024 22:17

Jason-Y-Z force-pushed the fix-issue-115957 branch from 7f28db0 to 8e9866e Compare February 28, 2024 20:41

Jason-Y-Z changed the title ~~gh-115957: Close coroutine if the TaskGroup has been closed~~ gh-115957: Close coroutine if the TaskGroup is inactive Feb 28, 2024

Jason-Y-Z force-pushed the fix-issue-115957 branch 2 times, most recently from 542d7c2 to e7c3a5e Compare March 2, 2024 11:30

gvanrossum requested changes Mar 5, 2024

View reviewed changes

bedevere-app bot added awaiting changes and removed awaiting review labels Mar 5, 2024

terryjreedy added the interpreter-core (Objects, Python, Grammar, and Parser dirs) label Mar 5, 2024

terryjreedy changed the title ~~gh-115957: Close coroutine if the TaskGroup is inactive~~ gh-115957: Close coroutine if the TaskGroup is inactive Mar 5, 2024

Jason-Y-Z and others added 3 commits March 5, 2024 08:51

pythongh-115957: Close coroutine if the TaskGroup is inactive

5bcc3d5

📜🤖 Added by blurb_it.

19c8bea

Catch warnings in inactive tg test and update docs

5d0d518

Jason-Y-Z force-pushed the fix-issue-115957 branch from 22d94f3 to 5d0d518 Compare March 5, 2024 09:21

bedevere-app bot added awaiting change review and removed awaiting changes labels Mar 5, 2024

bedevere-app bot requested a review from gvanrossum March 5, 2024 09:26

gvanrossum approved these changes Mar 6, 2024

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting change review labels Mar 6, 2024

gvanrossum merged commit ce0ae1d into python:main Mar 6, 2024
32 of 35 checks passed

bedevere-app bot removed the awaiting merge label Mar 6, 2024

adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 25, 2024

pythongh-115957: Close coroutine if TaskGroup.create_task() raises an…

258c243

… error (python#116009)

diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024

pythongh-115957: Close coroutine if TaskGroup.create_task() raises an…

d5be5e3

… error (python#116009)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-115957: Close coroutine if the TaskGroup is inactive #116009

gh-115957: Close coroutine if the TaskGroup is inactive #116009

Jason-Y-Z commented Feb 27, 2024 •

edited by terryjreedy

Loading

arthur-tacca commented Feb 28, 2024

Jason-Y-Z commented Feb 28, 2024

arthur-tacca commented Feb 28, 2024

Jason-Y-Z commented Feb 29, 2024

gvanrossum commented Mar 1, 2024

Jason-Y-Z commented Mar 1, 2024

gvanrossum left a comment

gvanrossum Mar 5, 2024

Jason-Y-Z Mar 5, 2024

gvanrossum Mar 5, 2024

Jason-Y-Z Mar 5, 2024

bedevere-app bot commented Mar 5, 2024

Jason-Y-Z commented Mar 5, 2024

bedevere-app bot commented Mar 5, 2024

gvanrossum left a comment

gvanrossum commented Mar 6, 2024

Jason-Y-Z commented Mar 7, 2024

		.. versionchanged:: 3.13

		Close the given coroutine if the task group is not active.

gh-115957: Close coroutine if the TaskGroup is inactive #116009

gh-115957: Close coroutine if the TaskGroup is inactive #116009

Conversation

Jason-Y-Z commented Feb 27, 2024 • edited by terryjreedy Loading

arthur-tacca commented Feb 28, 2024

Jason-Y-Z commented Feb 28, 2024

arthur-tacca commented Feb 28, 2024

Jason-Y-Z commented Feb 29, 2024

gvanrossum commented Mar 1, 2024

Jason-Y-Z commented Mar 1, 2024

gvanrossum left a comment

Choose a reason for hiding this comment

gvanrossum Mar 5, 2024

Choose a reason for hiding this comment

Jason-Y-Z Mar 5, 2024

Choose a reason for hiding this comment

gvanrossum Mar 5, 2024

Choose a reason for hiding this comment

Jason-Y-Z Mar 5, 2024

Choose a reason for hiding this comment

bedevere-app bot commented Mar 5, 2024

Jason-Y-Z commented Mar 5, 2024

bedevere-app bot commented Mar 5, 2024

gvanrossum left a comment

Choose a reason for hiding this comment

gvanrossum commented Mar 6, 2024

Jason-Y-Z commented Mar 7, 2024

Jason-Y-Z commented Feb 27, 2024 •

edited by terryjreedy

Loading